Skip to content

TargetDefinition.java - Code Quality Improvements #2058

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

TargetDefinition.java - Code Quality Improvements #2058

wants to merge 1 commit into from

Conversation

@vogella
Copy link
Contributor

@vogella vogella commented Oct 25, 2025

Optimizations:

  • Line 629-631: Eliminated unnecessary ArrayList creation and addAll call - changed from 3 lines to 1 line returning new ArrayList<>(Arrays.asList(collection))
  • Line 598: Modernized array creation using method reference NameVersionDescriptor[]::new
  • Line 610: Modernized array creation using method reference TargetBundle[]::new
  • Line 527: Modernized array creation using method reference TargetBundle[]::new
  • Line 996: Modernized array creation using method reference TargetFeature[]::new

Impact:

  • Cleaner, more concise code
  • Slightly better performance by avoiding unnecessary intermediate objects
  • Modern Java idioms make code easier to maintain
  • No API changes

@vogella
TargetDefinition.java - Code Quality Improvements
add2013 
  Optimizations:
  - Line 629-631: Eliminated unnecessary ArrayList creation and addAll
call - changed from 3 lines to 1 line returning new
ArrayList<>(Arrays.asList(collection))
  - Line 598: Modernized array creation using method reference
NameVersionDescriptor[]::new
  - Line 610: Modernized array creation using method reference
TargetBundle[]::new
  - Line 527: Modernized array creation using method reference
TargetBundle[]::new
  - Line 996: Modernized array creation using method reference
TargetFeature[]::new

  Impact:
  - Cleaner, more concise code
  - Slightly better performance by avoiding unnecessary intermediate
objects
  - Modern Java idioms make code easier to maintain
  - No API changes
@laeubi
Copy link
Contributor

laeubi commented Oct 25, 2025

@vogella if we like this on a broader scope can we please enable the coresponding cleanup actions? Or do you plan anything on this particular class to work on?

@github-actions
Copy link

github-actions bot commented Oct 25, 2025

Test Results

111 files   -   649  111 suites   - 649   21m 33s ⏱️ - 37m 50s
310 tests  - 3 321  306 ✅  - 3 272   4 💤  -  48  0 ❌  - 1 
864 runs   - 9 771  851 ✅  - 9 630  13 💤  - 140  0 ❌  - 1 

Results for commit add2013. ± Comparison against base commit dd6dca3.

This pull request removes 3321 tests. 
AllPDETests AllBundleModelTests BundleActivatorTestCase ‑ testAbsentHeader
AllPDETests AllBundleModelTests BundleActivatorTestCase ‑ testChangeExistingActivator
AllPDETests AllBundleModelTests BundleActivatorTestCase ‑ testGetActivator
AllPDETests AllBundleModelTests BundleActivatorTestCase ‑ testHeaderLength
AllPDETests AllBundleModelTests BundleActivatorTestCase ‑ testHeaderLengthWithWindowsDelimiter
AllPDETests AllBundleModelTests BundleActivatorTestCase ‑ testHeaderOffset1
AllPDETests AllBundleModelTests BundleActivatorTestCase ‑ testHeaderOffset2
AllPDETests AllBundleModelTests BundleActivatorTestCase ‑ testPresentHeader
AllPDETests AllBundleModelTests BundleActivatorTestCase ‑ testSetActivator
AllPDETests AllBundleModelTests BundleClasspathTestCase ‑ testAbsentHeader
…

@vogella
Copy link
Contributor Author

vogella commented Oct 25, 2025

@vogella if we like this on a broader scope can we please enable the coresponding cleanup actions? Or do you plan anything on this particular class to work on?

Yes, I plan to look at the timestamp issue. and usually that means I do the save improvements before. Enabling clean-up actions for the project is useful but not related to this change (which was not done via the JDT clean-up action, AFAIK they cannot do such changes). I leave that to later or someone else.

@laeubi
Copy link
Contributor

laeubi commented Oct 25, 2025

AFAIK they cannot do such changes). I leave that to later or someone else.

@jjohnstn can tell better but I'm quite sure I have seen one for "Modernized array creation using method reference" already.

Yes, I plan to look at the timestamp issue.

Then you look at the wrong place, this is happening in P2Utils and alike.

HannesWell
HannesWell reviewed Oct 25, 2025
View reviewed changes
ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetDefinition.java
ArrayList<TargetBundle> result = new ArrayList<>();
result.addAll(Arrays.asList(collection));
return result;
return new ArrayList<>(Arrays.asList(collection));
Copy link
Member

@HannesWell HannesWell Oct 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the list should not be modified and you are sure the array doesn't contain null elements, you could also use List.of().

Copy link
Contributor Author

@vogella vogella Oct 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if null can happen or not, so my proposal would be to leave it as it.

@vogella
Copy link
Contributor Author

vogella commented Oct 25, 2025

AFAIK they cannot do such changes). I leave that to later or someone else.

@jjohnstn can tell better but I'm quite sure I have seen one for "Modernized array creation using method reference" already.

Nice, if that exists. I suggest to add this to the nightly batch clean-ups in this case.

Yes, I plan to look at the timestamp issue.

Then you look at the wrong place, this is happening in P2Utils and alike.

IMHO it never harms to look at the general logic while digging.

@vogella
Copy link
Contributor Author

vogella commented Oct 25, 2025

Wrth build issues: Once I see green builds in PDE again, I will rebase this to see if he have real issues here or not.

laeubi
laeubi requested changes Oct 25, 2025
View reviewed changes
Copy link
Contributor

@laeubi laeubi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, if that exists. I suggest to add this to the nightly batch clean-ups in this case.

Then please do so, we already have a lot of hassle at PDE with the JUnit 6 update I don't want to need review random "optimizations" done by another tool now without any added benefits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HannesWell HannesWell HannesWell left review comments

@laeubi laeubi laeubi requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vogella @laeubi @HannesWell